Merged
Conversation
evilwb
approved these changes
Mar 23, 2025
Owner
evilwb
left a comment
There was a problem hiding this comment.
This is an elegant solution indeed. Code looks good to me. Haven't tested myself but will go ahead and approve it. I assume that you've tested the basics like that collectables are still tracked correctly.
Collaborator
Author
|
I did a full run with the final change, including some stress tests on saves where I loaded back auto-saves and manuel saves on purpose, and it worked as expected. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes the auto-save issues, namely:
This was all caused by an issue in how saves are handled within the game, which is explained in detail in the issue linked above.
The solution that was picked is to move data around so that it is located in places that are coherent with how we want them saved:
Moving these variables around required some modifications in
Container.pyfor the game code to read these variables at the right address dynamically (usingMIPS.get_address_halves).For the 4 "global" variables, my first plan was to put it inside the "current ammo table" since it has 4 bytes per equipment and lots of them don't even use ammo. I did a test run with all 4 values packed into equipment 0x00 ammo, and for some reason I encountered several weird crashes I had never seen before. I suspected modifying the higher half of address has impacts, so I investigated but saw nothing that could explain it.
I tried moving those values to secondary inventory, did a test run, and everything worked perfectly fine, so I went with that solution instead.
Later, we'll need to add more custom values like those, so we'll need to dig deeper as to why the ammo count solution didn't work. Trying to put it as ammo for unused weapons or other equipement instead of equipment 0x00 might be a good start, but for now this fix is both sufficient and pretty elegant in my opinion.